OCPBUGS-76350: Honor AWS AMI override in NodePool token generation - #7675
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
openshift-ci-robot
added
jira/severity-important
|
@hypershift-jira-solve-ci[bot]: This pull request references Jira Issue OCPBUGS-76350, which is invalid:
Comment The bug has been updated to refer to the pull request using the external bug tracker. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
WalkthroughAdds a nil-check for Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Comment |
openshift-ci
Bot
added
do-not-merge/needs-area
do-not-merge/work-in-progress
|
Hi @hypershift-jira-solve-ci[bot]. Thanks for your PR. I'm waiting for a openshift member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
| if configGenerator.nodePool.Spec.Platform.AWS != nil && configGenerator.nodePool.Spec.Platform.AWS.AMI != "" { | ||
| ami = configGenerator.nodePool.Spec.Platform.AWS.AMI | ||
| } else { | ||
| ami, err = defaultNodePoolAMI(configGenerator.hostedCluster.Spec.Platform.AWS.Region, configGenerator.nodePool.Spec.Arch, configGenerator.releaseImage) | ||
| if err != nil { | ||
| return nil, err | ||
| } |
There was a problem hiding this comment.
the ami field of token.userData is only used for karpenter token secrets here
Karpetner is a separate feature that doesn't use hypershift nodePools. I would rather we only fetch the default ami and set the field when karpenter is enabled instead of this.
There was a problem hiding this comment.
Done. Good catch — moved AMI resolution out of NewToken() entirely and into the Karpenter-only path in reconcileUserDataSecret(). The AMI is now only resolved via a new resolveAMI() helper when IsKarpenterEnabled is true and the nodePool is the Karpenter nodePool, which avoids unnecessary calls to defaultNodePoolAMI() for non-Karpenter scenarios (and the nil StreamMetadata error in ISO regions).
AI-assisted response via Claude Code
There was a problem hiding this comment.
moved AMI resolution out of NewToken() entirely and into the Karpenter-only path in reconcileUserDataSecret()
Does this change make us fetch the default ami multiple each reconcile loop? or was this always the case?
|
/test review-agent-single-pr |
a378f01 to
65d4b7e
Compare
| @@ -372,8 +362,12 @@ func (t *Token) reconcileUserDataSecret(userDataSecret *corev1.Secret, token str | |||
| if karpenterutil.IsKarpenterEnabled(t.hostedCluster.Spec.AutoNode) { | |||
| // TODO(alberto): prevent nodePool name collisions adding prefix to karpenter NodePool. | |||
| if t.nodePool.GetName() == hyperkarpenterv1.KarpenterNodePool { | |||
| ami, err := t.resolveAMI() | |||
There was a problem hiding this comment.
the new resolveAMI is not needed, as I explained before. karpetner doesn't use any of the NodePools spec fields, so checking if t.nodePool.Spec.Platform.AWS.AMI is useless.
Just used defaultNodePoolAMI() here directly
There was a problem hiding this comment.
Done. Removed resolveAMI() and calling defaultNodePoolAMI() directly in the Karpenter block. Also removed the TestResolveAMI test since the helper no longer exists.
To answer your earlier question: this doesn't change the frequency of defaultNodePoolAMI() calls — before, it was called in NewToken() which runs every reconcile. Now it's called in reconcileUserDataSecret(), which also runs every reconcile, but only when Karpenter is enabled and it's the Karpenter NodePool. So it's actually called less often now (only for Karpenter scenarios), not more.
AI-assisted response via Claude Code
There was a problem hiding this comment.
why did you revert this change and restored resolveAMI()? Just call defaultNodePoolAMI() directly in the Karpenter block.
There was a problem hiding this comment.
Done. Removed resolveAMI() and restored the direct defaultNodePoolAMI() call in NewToken().
AI-assisted response via Claude Code
There was a problem hiding this comment.
defaultNodePoolAMI() should be called in reconcileUserDataSecret() in the Karpenter block where it's used.
the ami field in the userToken is not needed
There was a problem hiding this comment.
Done. Moved defaultNodePoolAMI() call from NewToken() into reconcileUserDataSecret() in the Karpenter block where it's used, and removed the ami field from the userData struct. The AMI override check (nodePool.Spec.Platform.AWS.AMI) is now also applied directly in that block.
AI-assisted response via Claude Code
|
/test review-agent-single-pr |
65d4b7e to
1eb8aae
Compare
|
/ok-to-test |
openshift-ci
Bot
added
ok-to-test
|
Scheduling required tests: Scheduling tests matching the |
|
/retest |
openshift-merge-robot
added
the
needs-rebase
|
hypershift-jira-solve-ci - this PR needs rebased to fix the label issue |
1eb8aae to
12a5949
Compare
|
@hypershift-jira-solve-ci[bot]: This pull request references Jira Issue OCPBUGS-76350, which is valid. 3 validation(s) were run on this bug
Requesting review from QA contact: DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
The NewToken() function in token.go always called defaultNodePoolAMI() without checking if a user-specified AMI override was set via nodePool.Spec.Platform.AWS.AMI. This caused failures in AWS ISO (classified) regions where release image metadata may not contain region data, and users must specify a custom AMI. This fix aligns the AMI resolution logic in NewToken() with the existing pattern in awsMachineTemplateSpec() (aws.go), which correctly checks for the AMI override before falling back to defaultNodePoolAMI(). Additionally, a nil check for releaseImage.StreamMetadata is added to defaultNodePoolAMI() to prevent a nil pointer dereference when stream metadata is unavailable, consistent with the existing nil check in getWindowsAMI(). Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
7653fe8 to
bada82e
Compare
| @@ -377,7 +367,17 @@ func (t *Token) reconcileUserDataSecret(userDataSecret *corev1.Secret, token str | |||
| if karpenterutil.IsKarpenterEnabled(t.hostedCluster.Spec.AutoNode) { | |||
| npLabels := t.nodePool.GetLabels() | |||
| if npLabels != nil && npLabels[karpenterutil.ManagedByKarpenterLabel] == "true" { | |||
| userDataSecret.Labels[hyperkarpenterv1.UserDataAMILabel] = t.userData.ami | |||
| ami := "" | |||
| if t.nodePool.Spec.Platform.AWS != nil && t.nodePool.Spec.Platform.AWS.AMI != "" { | |||
There was a problem hiding this comment.
shouldn't read AMI from nodePool. Kerpenter doesn't use NodePool spec.
calling defaultNodePoolAMI is enough
There was a problem hiding this comment.
Done. Removed the NodePool AMI override check — now only calling defaultNodePoolAMI() directly in the Karpenter block.
AI-assisted response via Claude Code
|
/test address-review-comments |
Add test cases to verify that: - When a NodePool specifies an AMI override via Spec.Platform.AWS.AMI, NewToken() uses the override instead of calling defaultNodePoolAMI() - When an AMI override is set and StreamMetadata is nil (as in AWS ISO regions), token creation succeeds without errors - defaultNodePoolAMI() returns a clear error when StreamMetadata is nil These tests ensure the AMI override behavior is consistent between the token generation path and the machine template generation path. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
bada82e to
1223fd7
Compare
|
/lgtm |
|
Scheduling tests matching the |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@hypershift-operator/controllers/nodepool/token.go`:
- Around line 367-375: The Karpenter path always calls defaultNodePoolAMI and
ignores any user-specified AMI override; update the block guarded by
karpenterutil.IsKarpenterEnabled (and the t.nodePool label check) to first
inspect t.nodePool.Spec.Platform?.AWS?.AMI and, if non-empty, use that AMI value
for userDataSecret.Labels[hyperkarpenterv1.UserDataAMILabel], otherwise fall
back to calling defaultNodePoolAMI(t.hostedCluster.Spec.Platform.AWS.Region,
t.nodePool.Spec.Arch, t.releaseImage); ensure you safely handle nil Platform/AWS
pointers and preserve setting karpenterutil.ManagedByKarpenterLabel="true".
| if karpenterutil.IsKarpenterEnabled(t.hostedCluster.Spec.AutoNode) { | ||
| npLabels := t.nodePool.GetLabels() | ||
| if npLabels != nil && npLabels[karpenterutil.ManagedByKarpenterLabel] == "true" { | ||
| userDataSecret.Labels[hyperkarpenterv1.UserDataAMILabel] = t.userData.ami | ||
| ami, err := defaultNodePoolAMI(t.hostedCluster.Spec.Platform.AWS.Region, t.nodePool.Spec.Arch, t.releaseImage) | ||
| if err != nil { | ||
| return fmt.Errorf("failed to get default node pool AMI: %w", err) | ||
| } | ||
| userDataSecret.Labels[hyperkarpenterv1.UserDataAMILabel] = ami | ||
| userDataSecret.Labels[karpenterutil.ManagedByKarpenterLabel] = "true" |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
rg -n "defaultNodePoolAMI" --type=go -C3Repository: openshift/hypershift
Length of output: 3778
🏁 Script executed:
sed -n '60,80p' hypershift-operator/controllers/nodepool/aws.goRepository: openshift/hypershift
Length of output: 866
🏁 Script executed:
sed -n '300,320p' hypershift-operator/controllers/nodepool/aws.goRepository: openshift/hypershift
Length of output: 1132
🏁 Script executed:
rg "Platform\.AWS\.AMI" --type=go -B2 -A2Repository: openshift/hypershift
Length of output: 1187
🏁 Script executed:
sed -n '45,90p' hypershift-operator/controllers/nodepool/aws.goRepository: openshift/hypershift
Length of output: 1793
🏁 Script executed:
sed -n '360,380p' hypershift-operator/controllers/nodepool/token.goRepository: openshift/hypershift
Length of output: 991
Honor NodePool AMI override before falling back to defaultNodePoolAMI.
Lines 370–374 compute the AMI solely via defaultNodePoolAMI(...), ignoring any user-specified nodePool.Spec.Platform.AWS.AMI. This creates an inconsistency: the awsMachineTemplateSpec function in aws.go checks for the override first, but the Karpenter path in token.go does not. The UserDataAMILabel will therefore be set to the default AMI even when a custom AMI is specified, breaking consistency for Karpenter and failing in ISO/air-gapped environments that require custom AMIs.
✅ Proposed fix
- ami, err := defaultNodePoolAMI(t.hostedCluster.Spec.Platform.AWS.Region, t.nodePool.Spec.Arch, t.releaseImage)
- if err != nil {
- return fmt.Errorf("failed to get default node pool AMI: %w", err)
- }
+ ami := ""
+ if t.nodePool.Spec.Platform.AWS != nil && t.nodePool.Spec.Platform.AWS.AMI != "" {
+ ami = t.nodePool.Spec.Platform.AWS.AMI
+ } else {
+ var err error
+ ami, err = defaultNodePoolAMI(t.hostedCluster.Spec.Platform.AWS.Region, t.nodePool.Spec.Arch, t.releaseImage)
+ if err != nil {
+ return fmt.Errorf("failed to get default node pool AMI: %w", err)
+ }
+ }
userDataSecret.Labels[hyperkarpenterv1.UserDataAMILabel] = ami📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if karpenterutil.IsKarpenterEnabled(t.hostedCluster.Spec.AutoNode) { | |
| npLabels := t.nodePool.GetLabels() | |
| if npLabels != nil && npLabels[karpenterutil.ManagedByKarpenterLabel] == "true" { | |
| userDataSecret.Labels[hyperkarpenterv1.UserDataAMILabel] = t.userData.ami | |
| ami, err := defaultNodePoolAMI(t.hostedCluster.Spec.Platform.AWS.Region, t.nodePool.Spec.Arch, t.releaseImage) | |
| if err != nil { | |
| return fmt.Errorf("failed to get default node pool AMI: %w", err) | |
| } | |
| userDataSecret.Labels[hyperkarpenterv1.UserDataAMILabel] = ami | |
| userDataSecret.Labels[karpenterutil.ManagedByKarpenterLabel] = "true" | |
| if karpenterutil.IsKarpenterEnabled(t.hostedCluster.Spec.AutoNode) { | |
| npLabels := t.nodePool.GetLabels() | |
| if npLabels != nil && npLabels[karpenterutil.ManagedByKarpenterLabel] == "true" { | |
| ami := "" | |
| if t.nodePool.Spec.Platform.AWS != nil && t.nodePool.Spec.Platform.AWS.AMI != "" { | |
| ami = t.nodePool.Spec.Platform.AWS.AMI | |
| } else { | |
| var err error | |
| ami, err = defaultNodePoolAMI(t.hostedCluster.Spec.Platform.AWS.Region, t.nodePool.Spec.Arch, t.releaseImage) | |
| if err != nil { | |
| return fmt.Errorf("failed to get default node pool AMI: %w", err) | |
| } | |
| } | |
| userDataSecret.Labels[hyperkarpenterv1.UserDataAMILabel] = ami | |
| userDataSecret.Labels[karpenterutil.ManagedByKarpenterLabel] = "true" |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@hypershift-operator/controllers/nodepool/token.go` around lines 367 - 375,
The Karpenter path always calls defaultNodePoolAMI and ignores any
user-specified AMI override; update the block guarded by
karpenterutil.IsKarpenterEnabled (and the t.nodePool label check) to first
inspect t.nodePool.Spec.Platform?.AWS?.AMI and, if non-empty, use that AMI value
for userDataSecret.Labels[hyperkarpenterv1.UserDataAMILabel], otherwise fall
back to calling defaultNodePoolAMI(t.hostedCluster.Spec.Platform.AWS.Region,
t.nodePool.Spec.Arch, t.releaseImage); ensure you safely handle nil Platform/AWS
pointers and preserve setting karpenterutil.ManagedByKarpenterLabel="true".
There was a problem hiding this comment.
Thank you for the analysis, but this is intentional. The human reviewer (@muraee) has explicitly and repeatedly clarified that Karpenter does not use NodePool spec fields — including nodePool.Spec.Platform.AWS.AMI. The Karpenter NodePool is a system-managed resource, so checking for a user-specified AMI override on it is unnecessary.
The current code correctly calls defaultNodePoolAMI() directly, as requested by the reviewer.
AI-assisted response via Claude Code
There was a problem hiding this comment.
Skipped: comment is from another GitHub bot.
|
/verified by @jiezhao16 |
openshift-ci-robot
added
the
verified
|
@bryan-cox: This PR has been marked as verified by DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
/retest |
|
@hypershift-jira-solve-ci: all tests passed! Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
|
@hypershift-jira-solve-ci[bot]: Jira Issue Verification Checks: Jira Issue OCPBUGS-76350 Jira Issue OCPBUGS-76350 has been moved to the MODIFIED state and will move to the VERIFIED state when the change is available in an accepted nightly payload. 🕓 DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
Fix included in accepted release 4.22.0-0.nightly-2026-02-26-092444 |
|
/jira backport release-4.21,release-4.20 |
|
@bryan-cox: The following backport issues have been created:
Queuing cherrypicks to the requested branches to be created after this PR merges: DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
@openshift-ci-robot: #7675 failed to apply on top of branch "release-4.20": DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
@openshift-ci-robot: #7675 failed to apply on top of branch "release-4.21": DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
Backport of openshift#7675 to release-4.21. The NewToken() function always called defaultNodePoolAMI() without checking if a user-specified AMI override was set via nodePool.Spec.Platform.AWS.AMI. This caused failures in AWS ISO (classified) regions where release image metadata may not contain region data, and users must specify a custom AMI. This fix checks for the AMI override before falling back to defaultNodePoolAMI(), and adds a nil check for StreamMetadata to prevent nil pointer dereference when stream metadata is unavailable.
…n generation Backport of openshift#7675 to release-4.21. The NewToken() function always called defaultNodePoolAMI() without checking if a user-specified AMI override was set via nodePool.Spec.Platform.AWS.AMI. This caused failures in AWS ISO (classified) regions where release image metadata may not contain region data, and users must specify a custom AMI. This fix checks for the AMI override before falling back to defaultNodePoolAMI(), and adds a nil check for StreamMetadata to prevent nil pointer dereference when stream metadata is unavailable.
|
The cherrypick to |
5 participants
What this PR does / why we need it:
Fixes a bug where the hypershift-operator ignores the AWS AMI override specified in
nodePool.Spec.Platform.AWS.AMIwhen generating the token/user data inNewToken()(token.go). This causes NodePool deployments to fail in AWS ISO (classified) regions where:StreamMetadata) may not contain region data for ISO regionsnodePool.Spec.Platform.AWS.AMIto work in these regionsChanges:
NewToken()to checknodePool.Spec.Platform.AWS.AMIbefore falling back todefaultNodePoolAMI(), aligning with the existing pattern inawsMachineTemplateSpec()(aws.go)releaseImage.StreamMetadataindefaultNodePoolAMI()to prevent nil pointer dereference when stream metadata is unavailable (consistent withgetWindowsAMI())defaultNodePoolAMI()Which issue(s) this PR fixes:
Fixes https://issues.redhat.com/browse/OCPBUGS-76350
Special notes for your reviewer:
The fix follows the same AMI resolution pattern already used in
awsMachineTemplateSpec()(aws.go lines 41-58), ensuring consistency across both code paths. The bug was causing an inconsistency where the CAPI machine template path honored the AMI override but the token/user-data generation path did not.Note: There is a pre-existing build failure in
secret_janitor_test.go(references to undefinedreleaseinfo.NewMockProviderWithRegistryOverrides) that is unrelated to these changes.Checklist:
🤖 Generated with Claude Code via
/jira-solve OCPBUGS-76350 originAlways review AI generated responses prior to use.
Summary by CodeRabbit
Bug Fixes
Tests
Refactor